Avoid access of front element in empty vectors #119
Conversation
|
Just want to mention that this patch makes lots more places in Boost.MPI benefit from the fix introduced in #81. |
|
I can confirm this PR fixes the assertion error blocking 1843105 on Fedora 33 for OpenMPI & MPICH. |
|
A test hangs, which is why the jobs time out; splitting them doesn't help with that. Add |
src/cartesian_communicator.cpp
Outdated
|
|
||
| namespace boost { namespace mpi { | ||
|
|
||
| namespace { | ||
| template <typename T, typename A> | ||
| T* c_data(std::vector<T,A>& v) { return &(v[0]); } | ||
| T* c_data(std::vector<T,A>& v) { return c_data(v); } |
There was a problem hiding this comment.
Isn't this going to call itself recursively forever?
| T* c_data(std::vector<T,A>& v) { return c_data(v); } | |
| T* c_data(std::vector<T,A>& v) { return detail::c_data(v); } |
There was a problem hiding this comment.
It might be simpler just to replace this function with using detail::c_data;
There was a problem hiding this comment.
Isn't this going to call itself recursively forever?
A legitimate concern.
There was a problem hiding this comment.
This not a recursive call, it's boost::mpi::<anonymous namespace>::c_data calling boost::mpi::detail::c_data. But I'll remove the former.
There was a problem hiding this comment.
How is it going to call detail::c_data? Name lookup inside the function finds that function itself, not one that isn't even visible.
There was a problem hiding this comment.
Ah sorry, I confused your suggested change with the diff. You are right of course.
|
The timeout were there before, e.g. #117 and all the other open PRs. Unfortunately there is no easy way to run the tests locally, so I think it's to cumbersome for me to debug this. |
|
Let me know if I should revert 1b2b7bb... |
I'll merge that one individually Thanks |
Can't reproduce: I've got all test passing on OpenMPI 4.0.0 and 4.0.4 (g++7.5.0) and Intel's 2020 compiler and MPI. And none of them took that long. |
There are multiple places in the library where a pointer to the memory managed by a vector by taking the address of the first element. This leads to undefined behavior if the vector is empty (eg there is no first element), which has caused problems downstream. For example some standard libraries have asserts for this. This patch is a modified version of a patch created by @mkuron which switches all the places that we found to
detail::c_data, which already existed in the library.(Please note that not all of the replacements are needed because in some places the vector can not
be empty, but I find the function call also more idiomatic and readable, and easier to replace when support for C++03 is dropped)